Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gh17060 gateway single host kube #17557

Merged
merged 50 commits into from
Aug 31, 2020
Merged

Gh17060 gateway single host kube #17557

merged 50 commits into from
Aug 31, 2020

Conversation

sparkoo
Copy link
Member

@sparkoo sparkoo commented Aug 4, 2020 •
edited
Loading

What does this PR do?

Implements Traefik based Gateway configuration on Kubernetes.

❗ This PR introduces one breaking change. This class https://github.com/eclipse/che/pull/17557/files#diff-8855fac8bb68d106dd25037d440a3e22 (previously was OpenShiftCheInstallationLocation https://github.com/eclipse/che/blob/master/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java) now fails when neither KUBERNETES_NAMESPACE or POD_NAMESPACE is set. Previously it returned null. The reason for it is that we need to know che installation namespace here and can't work without it so we should fail fast (at startup). The installers (che-operator, helm) are responsible to set one of these env variables. If there will be any failures, it should be fixed on their side.

What issues does this PR fix or reference?

#17060

Release Notes

Docs PR

@sparkoo
first treafik config generator implementation
a2bb8c9 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
refactor generating gateway config
56a3f8e 
Signed-off-by: Michal Vala <mvala@redhat.com>
@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Aug 4, 2020
sparkoo
sparkoo commented Aug 4, 2020
View reviewed changes
.../workspace/infrastructure/kubernetes/server/external/TraefikGatewayRouteConfigGenerator.java
StringWriter sw = new StringWriter();

try {
YAMLGenerator generator =
Copy link
Member Author

@sparkoo sparkoo Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about using YAMLGenerator. We could use ObjectMapper, but then we would need to create all objects in configuration yaml or use multiple levels of Map<String, Map<String, Map<....
So I don't think it's so bad... Or any better idea?

@sparkoo
fix TraefikGatewayConfigGeneratorTest
1417611 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
test GatewayServerExposer
5d3e625 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
nullsafer way how to get port
9e275c4 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo sparkoo changed the title Gh17060 gateway single host kube 🚧 Gh17060 gateway single host kube Aug 4, 2020
@sparkoo
Copy link
Member Author

sparkoo commented Aug 4, 2020

setting it back to WIP as getting workspace namespace into GatewayServerExposer might not be trivial task.

@sparkoo
create route configmaps in che namespace
3cfabf4 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
Copy link
Member Author

sparkoo commented Aug 6, 2020

I've updated the PR with creating Gateway route configmaps in che namespace. There are lot of stuff missing, but I'd like to know opinion about general idea here, especially GatewayRouterProvisioner.java, where I'm creating new KubernetesNamespace with "che" namespace so I can create ConfigMaps in "che" namespace ("che" is hardcoded now).

Biggest part missing is cleanup of "che" namespace. I'm afraid I'll have to again create KubernetesNamespace for "che" namespace and workspaceId and do cleanup on it.

Thoughts? Ideas for cleaner solution?

mshaposhnik
mshaposhnik reviewed Aug 6, 2020
View reviewed changes
.../main/java/org/eclipse/che/workspace/infrastructure/kubernetes/GatewayRouterProvisioner.java Outdated
.endMetadata()
.withData(gatewayRouteConfigGenerator.generate());

ConfigMap routeConfigMap = cheNamespace.configMaps().create(configMapBuilder.build());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I afraid you will face the permission problem accessing che namespace, which i have faced during search of CA configmap.

Copy link
Member Author

@sparkoo sparkoo Aug 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. I'm afraid we would have to somehow create that configmap in namespace with gateway pod. If we won't have permissions for that, we would have to add it. I'll look what permissions we have and if we need more. Alternatively, we could configure configbump to watch configmaps on all namespaces, or namespaces with running workspaces, which would require higher permissions on configbump SA.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be a big problem to require the che SA to be able to crud configmaps in the che namespace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it works with current helm install (we use cluster-admin). With operator, we currently can't use different namespaces anyway because lack of permissions and in one namespace it works fine.

So last option is OpenShift OAuth. I didn't try it, but this task is about Kubernetes so I would leave this potential permissions issue to #17061

skabashnyuk
skabashnyuk reviewed Aug 7, 2020
View reviewed changes
Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Biggest part missing is cleanup of "che" namespace. I'm afraid I'll have to again create KubernetesNamespace for "che" namespace and workspaceId and do cleanup on it.

Should we create something like CheServerKubernetesNamespace to encapsulate logic that might be needed che server to work with k8s object on his namespace?

I think direction is ok. I would like to hear @metlos and @sleshchenko option before the merge too.

.../main/java/org/eclipse/che/workspace/infrastructure/kubernetes/GatewayRouterProvisioner.java Outdated Show resolved Hide resolved
.../workspace/infrastructure/kubernetes/server/external/TraefikGatewayRouteConfigGenerator.java Outdated Show resolved Hide resolved
@sparkoo
Copy link
Member Author

sparkoo commented Aug 7, 2020

Should we create something like CheServerKubernetesNamespace to encapsulate logic that might be needed che server to work with k8s object on his namespace?

I think it's worth to have @Singleton for it so we don't have to create it everytime. However, we can't reuse much code from existing KubernetesNamespace as all that bean is bound to single workspace. I'll try to come up with some code, so we can have more focused discussion.

@sparkoo
create gateway configmaps in che namespace
e0ec48c 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
gateway prefix path without slash
641d6ae 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
Merge branch 'master' into gh17060-gatewaySingleHostKube
eb28dce
@sparkoo
cleanup
2fcd61d 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
revert unnecessary change
ee77b6b 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
license headers
83e4774 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
fix NPE when checking object to create in che installation namespace
c77a4ee 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
javadocs
8f1c14e 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
Copy link
Member Author

sparkoo commented Aug 20, 2020

@amisevsk I've created an issue to reduce the number of configmaps #17669

@metlos Code is now refactored so we use only ConfigMaps, which are provisioned in first phase and "materialized" together with others.

@sparkoo
rename CheKubernetesClientFactory -> CheServerKubernetesClientFactory…
4d6337c 
… and move method to check che namespace annotation into util class

Signed-off-by: Michal Vala <mvala@redhat.com>
@eclipse-che eclipse-che deleted a comment from che-bot Aug 21, 2020
@eclipse-che eclipse-che deleted a comment from che-bot Aug 21, 2020
@eclipse-che eclipse-che deleted a comment from che-bot Aug 21, 2020
@eclipse-che eclipse-che deleted a comment from che-bot Aug 21, 2020
@eclipse-che eclipse-che deleted a comment from che-bot Aug 21, 2020
@eclipse-che eclipse-che deleted a comment from che-bot Aug 21, 2020
@sparkoo
expose paths with ending slash
ec1201d 
Signed-off-by: Michal Vala <mvala@redhat.com>
@eclipse-che eclipse-che deleted a comment from che-bot Aug 21, 2020
metlos
metlos reviewed Aug 24, 2020
View reviewed changes
Copy link
Contributor

@metlos metlos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the overall approach is good. My only comment would be that the gateway provisioning is not encapsulated too well, because the fact that we're using configmaps to "materialize" the gateway config is omnipresent. I would imagine that KubernetesInternalRuntime would be completely unwaware of what the individual provisioners "do" to the environment yet we assume there that some configmaps might be deployed to the che namespace, which seems to me like a quite concrete assumption.

That said, I don't consider the above to be a blocker - we might abstract this if/when the need arises.

@sparkoo
Copy link
Member Author

sparkoo commented Aug 24, 2020 •
edited
Loading

I would imagine that KubernetesInternalRuntime would be completely unwaware of what the individual provisioners "do" to the environment yet we assume there that some configmaps might be deployed to the che namespace, which seems to me like a quite concrete assumption.

Yes, but KubernetesInternalRuntime is responsible to materialize the objects so it has to know how and where to create them. I can imagine to abstract what is now namespace and cheNamespace in KubernetesInternalRuntime with one layer that will decide where to create the objects. But I don't think it's worth it at this point.

@rhopp rhopp mentioned this pull request Aug 25, 2020
Closed
@sparkoo
Merge branch 'master' into gh17060-gatewaySingleHostKube
fe6e99e 
# Conflicts:
#	infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesEnvironmentProvisionerTest.java
@sparkoo sparkoo merged commit 7c4d0a8 into eclipse-che:master Aug 31, 2020
@sparkoo sparkoo deleted the gh17060-gatewaySingleHostKube branch August 31, 2020 08:51
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Aug 31, 2020
@che-bot che-bot added this to the 7.19 milestone Aug 31, 2020
@sparkoo sparkoo mentioned this pull request Sep 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metlos metlos metlos approved these changes

@skabashnyuk skabashnyuk skabashnyuk approved these changes

@l0rd l0rd Awaiting requested review from l0rd

@nickboldt nickboldt Awaiting requested review from nickboldt

@sleshchenko sleshchenko Awaiting requested review from sleshchenko

@mshaposhnik mshaposhnik Awaiting requested review from mshaposhnik

@amisevsk amisevsk Awaiting requested review from amisevsk

Assignees
No one assigned
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Milestone
7.19
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@sparkoo @amisevsk @metlos @skabashnyuk @mshaposhnik @che-bot